Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce check for dangling RAA blockers in test_utils #3504

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

shaavan
Copy link
Member

@shaavan shaavan commented Jan 3, 2025

Partially addresses #3381

Summary

This PR addresses the issue of unhandled dangling RAA blockers during testing by introducing a check in Node::Drop().

Changes:

  1. Added a validation in Node::Drop() to ensure no dangling RAA blockers remain unhandled.
  2. Updated the test to handle RAA blockers, fixing the failure caused by the new validation.

@shaavan
Copy link
Member Author

shaavan commented Jan 8, 2025

Updated from pr3504.01 to pr3504.02 (diff):
Addressed @jkczyz comments

Changes:

  1. Updated get_and_clear_raa_blockers to pub(crate) to eliminate redundant test-specific declarations for other structs.

Copy link

codecov bot commented Jan 8, 2025

Codecov Report

Attention: Patch coverage is 91.04478% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.42%. Comparing base (aa2c6fe) to head (011e847).

Files with missing lines Patch % Lines
lightning/src/ln/channelmanager.rs 78.57% 1 Missing and 2 partials ⚠️
lightning/src/ln/chanmon_update_fail_tests.rs 95.91% 2 Missing ⚠️
lightning/src/ln/functional_test_utils.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3504      +/-   ##
==========================================
- Coverage   88.44%   88.42%   -0.02%     
==========================================
  Files         149      149              
  Lines      113330   113383      +53     
  Branches   113330   113383      +53     
==========================================
+ Hits       100238   100264      +26     
- Misses      10621    10636      +15     
- Partials     2471     2483      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jkczyz jkczyz requested review from TheBlueMatt and valentinewallace and removed request for TheBlueMatt January 9, 2025 22:58
@TheBlueMatt
Copy link
Collaborator

Nice, thanks!

I'd rather we fix the tests that currently leave dangling RAA blockers than allow them. This patch as a prefactor to yours should do it:

diff --git a/lightning/src/ln/chanmon_update_fail_tests.rs b/lightning/src/ln/chanmon_update_fail_tests.rs
index 590636e0d..00fd9501c 100644
--- a/lightning/src/ln/chanmon_update_fail_tests.rs
+++ b/lightning/src/ln/chanmon_update_fail_tests.rs
@@ -3311,41 +3311,30 @@ fn do_test_durable_preimages_on_closed_channel(close_chans_before_reload: bool,
 		reconnect_args.pending_raa.1 = true;

 		reconnect_nodes(reconnect_args);
+	}

-		// Once the blocked `ChannelMonitorUpdate` *finally* completes, the pending
-		// `PaymentForwarded` event will finally be released.
-		let (outpoint, ab_update_id, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id_ab).unwrap().clone();
-		nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, ab_update_id);
+	// Once the blocked `ChannelMonitorUpdate` *finally* completes, the pending
+	// `PaymentForwarded` event will finally be released.
+	let (outpoint, ab_update_id, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id_ab).unwrap().clone();
+	nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, ab_update_id);

-		// If the A<->B channel was closed before we reload, we'll replay the claim against it on
-		// reload, causing the `PaymentForwarded` event to get replayed.
-		let evs = nodes[1].node.get_and_clear_pending_events();
-		assert_eq!(evs.len(), if close_chans_before_reload { 2 } else { 1 });
-		for ev in evs {
-			if let Event::PaymentForwarded { .. } = ev { }
-			else {
-				panic!();
-			}
+	// If the A<->B channel was closed before we reload, we'll replay the claim against it on
+	// reload, causing the `PaymentForwarded` event to get replayed.
+	let evs = nodes[1].node.get_and_clear_pending_events();
+	assert_eq!(evs.len(), if close_chans_before_reload { 2 } else { 1 });
+	for ev in evs {
+		if let Event::PaymentForwarded { .. } = ev { }
+		else {
+			panic!();
 		}
+	}

+	if !close_chans_before_reload || close_only_a {
 		// Once we call `process_pending_events` the final `ChannelMonitor` for the B<->C channel
 		// will fly, removing the payment preimage from it.
 		check_added_monitors(&nodes[1], 1);
 		assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 		send_payment(&nodes[1], &[&nodes[2]], 100_000);
-	} else {
-		// Handle RAA blockers on nodes[1]
-		let raa_blockers = nodes[1].node.get_and_clear_pending_raa_blockers();
-		assert_eq!(raa_blockers.len(), 1);
-		let (chan_id, blockers) = &raa_blockers[0];
-		assert_eq!(*chan_id, chan_id_bc);
-		assert_eq!(blockers.len(), 1);
-		match blockers[0] {
-			RAAMonitorUpdateBlockingAction::ForwardedPaymentInboundClaim { channel_id, .. } => {
-				assert_eq!(channel_id, chan_id_ab);
-			}
-			_ => panic!("Unexpected RAA blocker")
-		}
 	}
 }

@@ -3561,6 +3550,9 @@ fn do_test_glacial_peer_cant_hang(hold_chan_a: bool) {
 	let node_chanmgrs = create_node_chanmgrs(3, &node_cfgs, &[None, None, None]);
 	let mut nodes = create_network(3, &node_cfgs, &node_chanmgrs);

+	let node_a_id = nodes[0].node.get_our_node_id();
+	let node_c_id = nodes[2].node.get_our_node_id();
+
 	let chan_id_ab = create_announced_chan_between_nodes(&nodes, 0, 1).2;
 	let chan_id_bc = create_announced_chan_between_nodes(&nodes, 1, 2).2;

@@ -3574,7 +3566,7 @@ fn do_test_glacial_peer_cant_hang(hold_chan_a: bool) {

 	let cs_updates = get_htlc_update_msgs(&nodes[2], &nodes[1].node.get_our_node_id());
 	if hold_chan_a {
-		// The first update will be on the A <-> B channel, which we allow to complete.
+		// The first update will be on the A <-> B channel, which we optionally allow to complete.
 		chanmon_cfgs[1].persister.set_update_ret(ChannelMonitorUpdateStatus::InProgress);
 	}
 	nodes[1].node.handle_update_fulfill_htlc(nodes[2].node.get_our_node_id(), &cs_updates.update_fulfill_htlcs[0]);
@@ -3601,8 +3593,11 @@ fn do_test_glacial_peer_cant_hang(hold_chan_a: bool) {
 		assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 		assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

-		let (route, payment_hash_2, _, payment_secret_2) = get_route_and_payment_hash!(&nodes[1], nodes[2], 1_000_000);
+		let (route, payment_hash_2, payment_preimage_2, payment_secret_2) =
+			get_route_and_payment_hash!(&nodes[1], nodes[2], 1_000_000);

+		// With the A<->B preimage persistence not yet complete, the B<->C channel is stuck
+		// waiting.
 		nodes[1].node.send_payment_with_route(route, payment_hash_2,
 			RecipientOnionFields::secret_only(payment_secret_2), PaymentId(payment_hash_2.0)).unwrap();
 		check_added_monitors(&nodes[1], 0);
@@ -3610,18 +3605,39 @@ fn do_test_glacial_peer_cant_hang(hold_chan_a: bool) {
 		assert!(nodes[1].node.get_and_clear_pending_events().is_empty());
 		assert!(nodes[1].node.get_and_clear_pending_msg_events().is_empty());

-		// Handle RAA blockers on nodes[1]
-		let raa_blockers = nodes[1].node.get_and_clear_pending_raa_blockers();
-		assert_eq!(raa_blockers.len(), 1);
-		let (chan_id, blockers) = &raa_blockers[0];
-		assert_eq!(*chan_id, chan_id_bc);
-		assert_eq!(blockers.len(), 1);
-		match blockers[0] {
-			RAAMonitorUpdateBlockingAction::ForwardedPaymentInboundClaim { channel_id, .. } => {
-				assert_eq!(channel_id, chan_id_ab);
+		// ...but once we complete the A<->B channel preimage persistence, the B<->C channel
+		// unlocks and we send both peers commitment updates.
+		let (outpoint, ab_update_id, _) = nodes[1].chain_monitor.latest_monitor_update_id.lock().unwrap().get(&chan_id_ab).unwrap().clone();
+		nodes[1].chain_monitor.chain_monitor.force_channel_monitor_updated(outpoint, ab_update_id);
+
+		let mut msg_events = nodes[1].node.get_and_clear_pending_msg_events();
+		assert_eq!(msg_events.len(), 2);
+		check_added_monitors(&nodes[1], 2);
+
+		let mut c_update = msg_events.iter()
+			.filter(|ev| matches!(ev, MessageSendEvent::UpdateHTLCs { node_id, .. } if *node_id == node_c_id))
+			.cloned().collect::<Vec<_>>();
+		let a_filtermap = |ev| if let MessageSendEvent::UpdateHTLCs { node_id, updates } = ev {
+			if node_id == node_a_id {
+				Some(updates)
+			} else {
+				None
 			}
-			_ => panic!("Unexpected RAA blocker")
-		}
+		} else {
+			None
+		};
+		let a_update = msg_events.drain(..).filter_map(|ev| a_filtermap(ev)).collect::<Vec<_>>();
+
+		assert_eq!(a_update.len(), 1);
+		assert_eq!(c_update.len(), 1);
+
+		nodes[0].node.handle_update_fulfill_htlc(nodes[1].node.get_our_node_id(), &a_update[0].update_fulfill_htlcs[0]);
+		commitment_signed_dance!(nodes[0], nodes[1], a_update[0].commitment_signed, false);
+		expect_payment_sent(&nodes[0], payment_preimage, None, true, true);
+		expect_payment_forwarded!(nodes[1], nodes[0], nodes[2], Some(1000), false, false);
+
+		pass_along_path(&nodes[1], &[&nodes[2]], 1_000_000, payment_hash_2, Some(payment_secret_2), c_update.pop().unwrap(), true, None);
+		claim_payment(&nodes[1], &[&nodes[2]], payment_preimage_2);
 	}
 }

@shaavan
Copy link
Member Author

shaavan commented Jan 20, 2025

Updated from pr3504.02 to pr3504.03 (diff):
Addressed @TheBlueMatt comments

Changes:

  1. Updated test to handle the dangling RAA blockers instead of catching them.

Thank you so much, @TheBlueMatt, for the help with the patch! :)

Note:
The `actions_blocking_raa_monitor_updates` list may contain stale entries
in the form of `(channel_id, [])`, which do not represent actual dangling actions.

To handle this, stale entries are ignored when accumulating pending actions
before clearing them. This ensures that the logic focuses only on relevant
actions and avoids unnecessary accumulation of already processed data.
@shaavan
Copy link
Member Author

shaavan commented Jan 20, 2025

Updated from pr3504.03 to pr3504.04 (diff):

Changes:

  1. Rebase on main to fix merge conflicts

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, modulo val's comment. Thanks!

@shaavan
Copy link
Member Author

shaavan commented Jan 25, 2025

Updated from pr3504.04 to pr3504.05 (diff):
Addressed @valentinewallace comment

Changes:

  1. Use public API channel_monitor_update over force_channel_monitor_update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants